Skip to content

[APPS-2792] Add: local-execution resilience tests (Milestone 7) - #492

Draft
tyffical wants to merge 30 commits into
tiffany.trinh/apps-2792-wire-into-dev-serverfrom
tiffany.trinh/apps-2792-local-execution-resilience-tests
Draft

[APPS-2792] Add: local-execution resilience tests (Milestone 7)#492
tyffical wants to merge 30 commits into
tiffany.trinh/apps-2792-wire-into-dev-serverfrom
tiffany.trinh/apps-2792-local-execution-resilience-tests

Conversation

@tyffical

@tyffical tyffical commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Motivation

  • Milestone 7 in the Local Node Execution Kickoff doc — renamed/descoped from "Chaos-engineering resilience testing" since domains/chaos-engineering targets Datadog-owned Kubernetes clusters, a mismatch for this in-process, customer-dev-server model.
  • Two targeted resilience checks, not fixes: empirically confirm the actual failure modes of the RFC's "no process isolation" decision, rather than leave them as assumed-but-unverified.

Changes

What changed File
New test confirming a synchronous CPU-bound loop starves the event loop, so the current Promise.race timeout never gets a chance to fire — it can only settle once the loop finishes on its own. local-execution.resilience.test.ts
New test (via a real child process, since process.exit() can't safely run inside this same Jest process) confirming process.exit() inside the customer function terminates the whole process immediately, bypassing try/finally cleanup entirely. local-execution.resilience.test.ts

The third Milestone 7 task ("the queue survives a rejected execution and keeps running") was already covered by an existing test in local-execution.test.ts, per the Kickoff doc's own checklist.

QA Instructions

yarn workspace @dd/tests test:unit packages/plugins/apps/src/vite/local-execution.resilience.test.ts
# Expected: 2 passed ✅ VERIFIED
yarn workspace @dd/tests test:unit packages/plugins/apps
# Expected: 25 suites / 324 tests passed ✅ VERIFIED
yarn workspace @dd/apps-plugin run typecheck
# Expected: clean exit ✅ VERIFIED

Blast Radius

  • Test-only change, no production code touched. Zero effect on shipping behavior.
  • Risk: none.

Out of Scope / Follow-ups

Item Status Next step
Whether to pursue real process/thread isolation (e.g. pooled worker_threads) to close the confirmed sync-hang and process.exit() gaps deferred These tests exist specifically to inform that decision with real data rather than speculation — a follow-up design discussion, not blocking this PR.

Documentation

@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-wire-into-dev-server branch 2 times, most recently from c2676bc to db2d170 Compare August 24, 2026 16:47
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-local-execution-resilience-tests branch 4 times, most recently from c994af1 to fe1ce1f Compare August 24, 2026 20:09
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-wire-into-dev-server branch from ce94528 to d02d8bd Compare August 25, 2026 00:23
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-local-execution-resilience-tests branch from fe1ce1f to 8777af3 Compare August 25, 2026 00:25
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-wire-into-dev-server branch from d02d8bd to d1d88d6 Compare August 25, 2026 01:09
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-local-execution-resilience-tests branch from 8777af3 to 91844e5 Compare August 25, 2026 01:10
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-wire-into-dev-server branch from d1d88d6 to 813061b Compare August 25, 2026 04:34
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-local-execution-resilience-tests branch from 91844e5 to 8fa667c Compare August 25, 2026 15:27
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-wire-into-dev-server branch from 96a5e43 to 18c9216 Compare August 25, 2026 16:57
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-local-execution-resilience-tests branch from 8fa667c to 28fb51c Compare August 26, 2026 01:51
@datadog-prod-us1-6

datadog-prod-us1-6 Bot commented Aug 26, 2026

Copy link
Copy Markdown

Tests

All CI checks and tests passed.

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 2afe0db | Docs | View more details | Give us feedback!

@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-wire-into-dev-server branch 3 times, most recently from 5b24d84 to 570261e Compare August 26, 2026 02:31
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-local-execution-resilience-tests branch from 28fb51c to 7805ce2 Compare August 26, 2026 02:39
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-wire-into-dev-server branch from 235b1a5 to bced83c Compare August 26, 2026 04:28
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-local-execution-resilience-tests branch from 7805ce2 to 7f5e32e Compare August 26, 2026 04:33
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-wire-into-dev-server branch from bced83c to b823f05 Compare August 26, 2026 15:42
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-local-execution-resilience-tests branch from 7f5e32e to a2d89c8 Compare August 26, 2026 16:17
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-wire-into-dev-server branch from b823f05 to 68e0026 Compare August 26, 2026 17:28
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-local-execution-resilience-tests branch from a2d89c8 to a110cb0 Compare August 26, 2026 17:32
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-wire-into-dev-server branch from 68e0026 to 60e9fcc Compare August 26, 2026 17:56
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-local-execution-resilience-tests branch from a110cb0 to 84beda0 Compare August 26, 2026 18:10
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-wire-into-dev-server branch 3 times, most recently from 959d0e8 to d3707a3 Compare August 27, 2026 16:44
Serializes concurrent executions to prevent one call's globalThis.$/registration
state from leaking into another, gives each execution its own $.Source object,
and closes confused-deputy and zombie-execution registration-poisoning gaps
where a completed or abandoned execution could still influence a later one's
action-catalog or apps-backend dispatch. Also treats .toJSON as a probed
property on the $.Actions proxy so JSON.stringify($) doesn't hang.
The stable Proxy wrapped every property access in a synthetic callable,
assuming the real @datadog/apps-backend runtime is a flat set of methods.
It isn't — e.g. user identity is a nested `.user.getExecutionUser()`
namespace — so any nested accessor threw "is not a function". Forward
each property straight through to the real, dispatch-cached runtime
instead.
…tale docs

Seeds globalDollarOutsideExecution from any globalThis.$ already installed
before this module loads (e.g. zx/globals), so installing the accessor
doesn't silently discard a pre-existing value. Replaces 4 remaining any-casts
in the test file with the existing testDollar() helper, and rewords 10
comments across local-execution.test.ts and execution-epoch.ts that still
described the removed poisoning mechanism or named consumer files that don't
exist yet.
… to {}

JSON.stringify(new Map(...)) and JSON.stringify(new Set(...)) both return
'{}' — a defined string, not undefined — so assertJsonSerializable's
existing undefined-check never caught them, silently dropping all of a
Map's/Set's entries instead of surfacing the same clear error given to
other non-serializable shapes (BigInt, functions, circular references).
…negative result

registerActionCatalogIfInstalled/registerBackendRuntimeIfInstalled cached
the 'not installed' outcome in the same WeakMap as a successful
registration, keyed by loadModule identity — a dev server reuses the same
loadModule for its whole lifetime, so once neither package was found, a
customer installing it mid-session (without restarting) got permanently
skipped instead of picked up on the next execution. The uncached
installedness check is a cheap require.resolve probe; only a *successful*
registration needs the once-ever WeakMap treatment.
JSON.stringify silently converts NaN/Infinity to "null" without
throwing, unlike every other non-serializable shape this check already
catches (Map/Set/BigInt/function/symbol) — a customer bug that produces
a non-finite result was returning a silent null instead of a clear,
attributed error.
Reading a runtime property directly off the proxy's target lost its
this-binding when called as backend.someMethod(), breaking any real
accessor that reads its own state via this instead of a closure. Also
distinguishes the apps-backend accessor's "no active execution" case
from "execution already concluded" the same way the action-catalog
dispatcher already does, instead of reporting a timeout that may not
have happened.
…cal-execution result

assertJsonSerializable only rejected a Map, Set, NaN, or Infinity at the top
level of a returned result. A JSON.stringify replacer runs on every key/value
pair it visits (root included), so checking there catches the same values
nested inside a plain object or array too, where JSON.stringify would
otherwise silently flatten them to "{}" or "null" instead of throwing.
testDollar()'s doc comment pointed at local-execution.ts's setGlobalDollar,
which no longer exists — globalThis.$ is now backed by an Object.defineProperty
accessor scoped through AsyncLocalStorage, not a plain get/set/delete helper trio.
…ater executions

A real dev server reuses the same loadModule for its whole process lifetime,
memoizing the action-catalog/apps-backend registration per loadModule identity.
If the underlying package load never settles (a broken/circular module graph,
not just a slow one), the cached promise stays pending forever, and every
later execution sharing that loadModule hangs on it until its own timeout —
never actually running its function, with no recovery short of a restart.
Bounding the load to the execution's own timeoutMs turns an unbounded hang
into a rejection, which the existing eviction-on-rejection logic already
handles correctly.
…tion

A customer module's own top-level evaluation runs before this execution's
box exists, and previously fell back to a plain undefined read instead of
failing the way a real Datadog deployment does at that same point. Also
reinstalls the accessor if a prior execution's customer code deleted
globalThis.$, so that deletion doesn't permanently break every later
execution in the same dev-server process.
…onSerializable's root call

The function/Symbol/undefined-drop check was exempted from the JSON root via
key === '', but a real object property can also be named the empty string
({ '': ... }) — that property silently lost its value the same way the
check exists to prevent, instead of throwing. Tracked via a one-shot flag
set on the replacer's first invocation instead, since JSON.stringify always
visits the root first regardless of its key.

Also un-inlines two loadModule/Promise.all calls passed directly into
withTimeout, per the repo's no-inlined-function-call-argument convention.
…lization and the epoch guard

Two doc comments described mechanisms that no longer match the code:
enqueue's own comment blamed a "shared module-level setter a concurrent
execution would clobber," but action-catalog/apps-backend registration is
now WeakMap-guarded and idempotent, so no concurrent execution clobbers it —
the real hazard enqueue guards against is a customer function deleting
globalThis.$ while another execution is still mid-flight. Separately, the
epoch guard's own comment framed it as a "belt-and-suspenders backstop"
redundant with enqueue's serialization, when it's actually the only thing
rejecting a timed-out execution's late dispatch during the overlap window
enqueue deliberately permits (the queue advances on timeout while the
abandoned fn() keeps running).

Also replaces two `as Error` casts in local-execution.test.ts with a
narrowing assertion helper, and fixes a UK spelling ("cancelled").
…n-epoch.ts

Comments added by this branch had grown into multi-sentence paragraphs
restating the same invariant several ways; compress each to one tight
sentence (two only for the few comments carrying a genuinely compound
invariant) without dropping the underlying WHY.
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-wire-into-dev-server branch from 6a6640b to dc85a6c Compare August 28, 2026 17:12
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-local-execution-resilience-tests branch from 13f8d38 to 04d8b27 Compare August 28, 2026 17:13
…ol-keyed results

globalThis.$ isn't a property at all in production until main() assigns
it, so an unresolvable $ reads as undefined per typeof's spec-defined
behavior on unresolvable references — it never throws. Locally, $ is a
real accessor property, so throwing from its getter broke that parity
for feature-detection code like `typeof $ !== 'undefined'`. Return
undefined instead when no execution or prior value has claimed $.

Also close a gap in assertJsonSerializable: JSON.stringify's replacer
is never invoked for a Symbol-KEYED property (only Symbol-valued ones
under a string key) — such properties were silently omitted with no
callback at all, defeating the "reject anything JSON.stringify would
silently drop" check. Added a dedicated recursive walk for this case.
rollupConfig.mjs's `external` list only matched a dependency's bare
specifier exactly, so `rollup/parseAst` (needed by the new dev-server
module-graph collector) got bundled instead of externalized despite
`rollup` being declared as a dependency. The bundled copy pulls in
Rollup's own native-binary platform loader, which throws through
`@rollup/plugin-commonjs`'s dynamic-require interop the moment a
consumer's real dev server calls it — breaking every published plugin
package for real users, not just this repo's own tests. Switched the
`external` option to a function that also matches subpath imports
(`id === name || id.startsWith(name + '/')`) for every declared
dependency and peer dependency.

Also drops a stray blank line introduced between a comment and the
test it documents in local-execution.test.ts.
The priming loadModule call (needed to populate the module graph for
connection-ID collection before the real execution starts) evaluates
the entry's real top-level code and runs before executeScriptLocally
installs its own hang-detection timeout — so it had no bound of its
own. A customer module with a hanging top-level await would wedge the
request forever. Wraps it in the same withTimeout helper the rest of
this file already uses, now exported for this cross-module use.
…rver output

The SSR transform Vite actually runs for a dev-server-only load rewrites every
import into a __vite_ssr_import__(...) call and resolves specifiers to
absolute paths, which the plain-ImportDeclaration AST search built for a real
Rollup build can't parse. Read each module's original source from disk and
strip TS/JSX with esbuild in isolation instead, so the parser sees the same
untransformed import syntax the production build path already trusts.
…ound total execution time

collectModuleGraphFromServer silently fell back to a static import's raw
specifier text when Vite's resolveId failed to resolve it, instead of
failing closed like every sibling module-graph error path — a
connectionId-scoped action call behind an unresolvable import would
silently drop out of the allowlist instead of the request failing loudly.

/__dd/executeAction ran a customer's real backend code with no auth check
upfront, only lazily inside a $.Actions call — unlike production, which
authenticates before any query/execution logic runs (app-builder-api's
PreviewAsyncQueryHandler). A function that never calls $.Actions was a
loophole around the same requirement production always enforces. Checked
upfront as a local credential-presence check, not a network call, so it
costs no latency on the local dev loop.

guardedExecuteAction's hang-detection pause can't distinguish a customer
function genuinely awaiting a slow $.Actions call from one that fired a
call without awaiting it and then hung on something unrelated — an
unawaited call masked a real hang for up to MAX_ACTION_CALL_TIMEOUT_MS (10
minutes). A second, independent absolute ceiling now bounds one
execution's total wall-clock time regardless of pendingActionCalls, set
just above pollQueryExecution's own ~300s worst-case long-poll budget so a
legitimate slow call still always finishes.

Also fixes a comment narrating this PR's own before/after history and an
embedded milestone number, both against repo convention.
makeExecuteActionRemotely's own inner auth check and doc comment
("no auth needed until a call is actually made") went stale once the
caller started requiring auth upfront — its only caller is only ever
reached after that upfront check already passed, making the inner
check unreachable and the comment actively contradictory. Narrows both
functions' doAuthenticatedRequest parameter to required, matching how
the sibling /__dd/executeActionViaCloud path already types it.

Removes the now-redundant "no auth + calls $.Actions" test, fully
subsumed by the upfront-check test right after it — no test exercises
the removed lazy check anymore since nothing can reach it.
…eScriptLocally's own load

handleExecuteAction's priming load is the only place a customer module's
top-level code actually runs (Vite caches the module, so
executeScriptLocally's own load below just reuses the resolved object) —
but it called loadModule directly instead of going through
customerModuleLoadContext, so a customer module reaching for $ during its
own top-level evaluation silently resolved to whatever $ a prior execution
left behind instead of throwing the same way it does inside
executeScriptLocally. Extracts the scoping into loadCustomerModuleEntry,
shared by both call sites.

Also corrects the startup warning logged when auth isn't configured: it
still described only $.Actions calls as failing, but the earlier
auth-upfront hardening rejects the whole /__dd/executeAction endpoint
before any backend-function code runs, regardless of whether it calls
$.Actions.
…violations found in round-6 review

- Add test coverage for the startup auth-warning log message (dev-server.ts)
  — no test asserted its exact wording, which is how a stale claim about
  which endpoints fail escaped an earlier review round.
- guardedExecuteAction is async, so its not-current branch's
  `return Promise.reject(...)` was needless wrapping — use `throw` instead.
- Extract three inlined function-call arguments (withTimeout's first
  argument at two dev-server.ts call sites, executeAction's result in
  local-execution.ts) into named locals, matching this file's own existing
  convention at every other withTimeout call site.
- Replace a bare `as any` cast (with an eslint-disable to suppress the rule
  that would flag it) in dev-server-module-graph.test.ts with a narrower
  `as unknown as ViteDevServer`.
- Move two same-line comments onto their own line.
- Reword four regression-test comments in dev-server.integration.test.ts
  that narrated "before the fix, X happened" — restated as the present-tense
  invariant each test guards.
…on.ts

Multi-paragraph and multi-clause comments compressed to one dense
sentence, or two-to-three for a genuinely complex invariant, keeping
every distinct WHY-reason without restating what the code already
shows.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant